Make threadsafe evaluation opt-in #1151
Conversation
penelopeysm
force-pushed
the
py/opt-in-tsvi
branch
from
54f1b7b to
b432bfc
Compare
| "It looks like you are using `Threads.@threads` in your model definition." * | ||
| "\n\nNote that since version 0.39 of DynamicPPL, threadsafe evaluation of models is disabled by default." * | ||
| " If you need it, you will need to explicitly enable it by creating the model, and then running `model = setthreadsafe(model, true)`." * | ||
| "\n\nAvoiding threadsafe evaluation can often lead to significant performance improvements. Please see https://turinglang.org/docs/THIS_PAGE_DOESNT_EXIST_YET for more details of when threadsafe evaluation is actually required." |
There was a problem hiding this comment.
I'm writing this now.
Benchmark Report
Computer InformationBenchmark Results |
penelopeysm
force-pushed
the
py/opt-in-tsvi
branch
from
b432bfc to
2369a5d
Compare
|
DynamicPPL.jl documentation for PR #1151 is available at: |
penelopeysm
force-pushed
the
py/opt-in-tsvi
branch
from
2369a5d to
f862acb
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #1151 +/- ##
============================================
- Coverage 81.67% 81.16% -0.52%
============================================
Files 42 42
Lines 3930 3934 +4
============================================
- Hits 3210 3193 -17
- Misses 720 741 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mhauru
left a comment
mhauru
left a comment
There was a problem hiding this comment.
Very happy with this. Only some minor style points to raise, and to wait for that docs page to be up.
| contextualize | ||
| ``` | ||
|
|
||
| Some models require threadsafe evaluation (see https://turinglang.org/docs/THIS_DOESNT_EXIST_YET for more information on when this is necessary). |
There was a problem hiding this comment.
Just a reminder note to change this before merging.
src/fasteval.jl
Outdated
| accs = map( | ||
| acc -> DynamicPPL.convert_eltype(float_type_with_fallback(eltype(params)), acc), | ||
| accs, | ||
| ) | ||
| vi_wrapped = ThreadSafeVarInfo(OnlyAccsVarInfo(accs)) | ||
| _, vi_wrapped = DynamicPPL._evaluate!!(model, vi_wrapped) | ||
| vi = OnlyAccsVarInfo(DynamicPPL.getaccs(vi_wrapped)) |
There was a problem hiding this comment.
Would it be simpler to have a single method and wrap this part in a if is_threaded(f.model)? It should get constant propagated away at compile time. Very optional to change, the current version isn't bad either.
This probably purely a style question, but there could be a difference in that listing all the type parameters of Model in the function signature I think forces specialisation on all of them.
There was a problem hiding this comment.
Yes, good idea, I was a bit concerned about the specialisation too. I thought about making threaded the first type parameter, which would also avoid this (and we don't rarely dispatch on any other type parameters in Model)... then decided against it because it might be too breaking
| args::NamedTuple{argnames,Targs}, | ||
| defaults::NamedTuple{kwargnames,Tkwargs}, | ||
| context::AbstractContext=DefaultContext(), | ||
| threadsafe::Bool=false, |
There was a problem hiding this comment.
Is there a particular meaning to sometimes calling this threadsafe and sometimes Threaded?
There was a problem hiding this comment.
Threaded is a type parameter and threadsafe is a function argument 😄
A bit like Ctx and context.
Open to different names though!
src/compiler.jl
Outdated
| # If it's a macro, we expand it | ||
| if Meta.isexpr(expr, :macrocall) | ||
| return generate_mainbody!(mod, found, macroexpand(mod, expr; recursive=true), warn) | ||
| if expr.args[1] == Expr(:., :Threads, QuoteNode(Symbol("@threads"))) && |
There was a problem hiding this comment.
I wonder how often people do
using Threads: @threads
@threadsvs how often people call some other macro called @threads. I.e. false positives vs false negatives.
There was a problem hiding this comment.
Another case where I wasn't too sure where to draw the line. I think Threads.@threads probably accounts for most usage, but I'm not averse to also handling @threads since after all the warning message is quite noncommittal.
There was a problem hiding this comment.
I would err on the safe side in warning about @threads, but happy if you prefer otherwise.
This adds a new type parameter to the model struct, which indicates whether threadsafe evaluation is required or not.
The default is false, so users must specifically opt-in. It's easy to make this opt-out instead, by changing the default to true. But that would probably tank performance on single-threaded sessions.
If a user declares a model with
Threads.@threadsinside, it will issue a warning. This is not a foolproof detection method. But it will probably catch 95% of cases.I believe this is the best technical solution. #1128 provides a global on/off switch, but that is quite slow. Furthermore, it's not technically correct either; threadsafe evaluation is a property of a model, not of a Julia session.
Here is a demo (launch Julia with > 1 thread, or else you won't observe this):
The above gives wrong results because threadsafe evaluation wasn't requested. This PR lets you do that: